Fix hide item option not working in Program plugin #4141
Conversation
Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
Introduced an optional `resetCache` parameter to `IndexWin32ProgramsAsync` and `IndexUwpProgramsAsync` to allow finer control over cache resets during indexing. Updated `IndexProgramsAsync` to explicitly pass `true` for cache resets. Modified `DisableProgramAsync` to return a `bool` indicating success and adjusted its logic to prevent unnecessary cache resets when reindexing. Ensured it returns `false` if the program is already disabled. Replaced nullable `PackageCatalog?` with non-nullable `PackageCatalog` in `UWPPackage.cs` to ensure proper initialization. Updated `WatchPackageChangeAsync` and `WatchDirectory` to call indexing methods with `resetCache` set to `true` when changes are detected. Improved cache reset logic across methods by conditionally invoking `ResetCache` based on the `resetCache` parameter, enhancing performance and maintainability.
prlabeler
bot
added
bug
|
🥷 Code experts: no user but you matched threshold 10 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRefactors Program plugin indexing and disable flow: indexing methods now accept a Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ContextMenu / UI
participant Main as Program.Main
participant Cache as MemoryCache
participant Indexer as Indexing (Win32/UWP)
UI->>Main: Request DisableProgram(program)
alt Program found
Main->>Main: Acquire lock, remove program from list
Main->>Indexer: Enqueue targeted reindex (resetCache: false)
Main->>Cache: ResetCache() (atomic swap)
Cache-->>Main: Old cache disposed
Main->>UI: Show success toast
UI->>Main: Re-query
else Program not found / already disabled
Main->>UI: Return false (no-op)
end
Note over Main,Indexer: UWP/Win32 watchers call Index*Async(resetCache: true) on package changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Plugins/Flow.Launcher.Plugin.Program/Main.cs(7 hunks)Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.csPlugins/Flow.Launcher.Plugin.Program/Programs/Win32.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-10-16T10:48:30.573Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.csPlugins/Flow.Launcher.Plugin.Program/Programs/Win32.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.csPlugins/Flow.Launcher.Plugin.Program/Programs/Win32.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-09-04T11:52:29.096Z
Learnt from: jjw24
Repo: Flow-Launcher/Flow.Launcher PR: 3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.096Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.csPlugins/Flow.Launcher.Plugin.Program/Programs/Win32.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs (2)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (8)
Task(81-162)Task(214-335)Task(337-366)Task(368-397)Task(399-412)Task(480-522)Task(539-542)Main(18-548)Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (1)
Task(295-323)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Agent
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (4)
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (1)
293-293: LGTM!The nullability change is appropriate since
catalogis always initialized via null-coalescing assignment (catalog ??= PackageCatalog.OpenForCurrentUser()) before any access withinWatchPackageChangeAsync.Plugins/Flow.Launcher.Plugin.Program/Main.cs (3)
337-366: LGTM!The conditional cache reset based on the
resetCacheparameter is well-implemented. The method properly acquires/releases the lock and handles exceptions.
368-397: LGTM!Consistent implementation with
IndexWin32ProgramsAsync. The conditional cache reset pattern is correct.
403-408: Timing measurement is correct; no issue found.The
StopwatchLogInfoAsyncmethod correctly awaits theFunc<Task>parameter at line 53 inFlow.Launcher.Infrastructure/Stopwatch.cs. Both() => IndexWin32ProgramsAsync(true)and the suggestedasync () => await IndexWin32ProgramsAsync(true)are functionally equivalent—they both return aTaskthat is properly awaited byInfoAsync, and the stopwatch accurately measures the actual indexing duration. The timing measurement works correctly as-is.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the program disabling and indexing logic in the Flow.Launcher Program plugin to fix an issue where the "hide item" option wasn't working properly. The changes introduce a resetCache parameter to indexing methods for better cache control, refactor the DisableProgramAsync method to return a boolean and trigger targeted reindexing, and improve error handling in the context menu action.
- Added a
resetCacheparameter to indexing methods to control when query cache is reset - Refactored
DisableProgramAsyncto return a boolean indicating success and trigger type-specific reindexing - Wrapped the disable program action in try-catch with proper error logging
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.Program/Main.cs | Added resetCache parameter to indexing methods; refactored DisableProgramAsync to return bool and trigger targeted reindexing; improved error handling in context menu action for disabling programs |
| Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs | Updated directory change monitor to explicitly pass resetCache: true when triggering Win32 program reindexing |
| Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs | Updated package change watcher to explicitly pass resetCache: true when triggering UWP program reindexing; changed catalog field from nullable to non-nullable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
The default value `resetCache = true` was removed from the method signatures of `IndexWin32ProgramsAsync` and `IndexUwpProgramsAsync` in the `Flow.Launcher.Plugin.Program` namespace. These methods now require the `resetCache` parameter to be explicitly provided by the caller. This change improves clarity and reduces ambiguity in method invocation.
|
@Jack251970 could you also include in the description what is the actual problem causing this issue. |
The root problem is the call of First() method. I fix this with null check |
Already added |
Updated method calls to replace boolean arguments with named parameters for better readability and self-documentation. - In `UWPPackage.cs`, replaced `true` with `resetCache: true` in `Main.IndexUwpProgramsAsync`. - In `Win32.cs`, replaced `true` with `resetCache: true` in `Main.IndexWin32ProgramsAsync`.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (1)
293-293: Nullable annotation should be preserved.The
catalogfield is not initialized at declaration and relies on the??=pattern at line 299 for lazy initialization. Removing the nullable annotation (?) creates a mismatch between the declared type and actual nullability state beforeWatchPackageChangeAsyncis called.- private static PackageCatalog catalog; + private static PackageCatalog? catalog;
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (2)
403-408: Consider using named parameters for consistency.Other call sites use
resetCache: truefor clarity. Consider applying the same pattern here for consistency across the codebase.- await Context.API.StopwatchLogInfoAsync(ClassName, "Win32Program index cost", () => IndexWin32ProgramsAsync(true)); + await Context.API.StopwatchLogInfoAsync(ClassName, "Win32Program index cost", () => IndexWin32ProgramsAsync(resetCache: true));- await Context.API.StopwatchLogInfoAsync(ClassName, "UWPProgram index cost", () => IndexUwpProgramsAsync(true)); + await Context.API.StopwatchLogInfoAsync(ClassName, "UWPProgram index cost", () => IndexUwpProgramsAsync(resetCache: true));
451-470: Consider adding user feedback when disable fails.The success message is now correctly shown only when a program is disabled. However, if
DisableProgramAsyncreturnsfalse(program already disabled or not found), the user receives no feedback. Consider showing an informational message in that case.var disabled = await DisableProgramAsync(program); if (disabled) { ResetCache(); Context.API.ShowMsg( Context.API.GetTranslation("flowlauncher_plugin_program_disable_dlgtitle_success"), Context.API.GetTranslation( "flowlauncher_plugin_program_disable_dlgtitle_success_message")); } + else + { + Context.API.ShowMsg( + Context.API.GetTranslation("flowlauncher_plugin_program_plugin_name"), + Context.API.GetTranslation("flowlauncher_plugin_program_disable_already_disabled")); + } Context.API.ReQuery();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Plugins/Flow.Launcher.Plugin.Program/Main.cs(7 hunks)Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.csPlugins/Flow.Launcher.Plugin.Program/Programs/Win32.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.csPlugins/Flow.Launcher.Plugin.Program/Programs/Win32.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-10-16T10:48:30.573Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.csPlugins/Flow.Launcher.Plugin.Program/Programs/Win32.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.csPlugins/Flow.Launcher.Plugin.Program/Programs/Win32.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-09-04T11:52:29.096Z
Learnt from: jjw24
Repo: Flow-Launcher/Flow.Launcher PR: 3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.096Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs
📚 Learning: 2025-11-06T12:57:11.574Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4072
File: Plugins/Flow.Launcher.Plugin.Program/Main.cs:100-111
Timestamp: 2025-11-06T12:57:11.574Z
Learning: In Flow Launcher's Program plugin Main.cs QueryAsync method, the semaphore pattern where WaitAsync(token) is inside a try-catch that only catches OperationCanceledException, followed by an explicit Release() call after the try-catch block, is safe and doesn't cause semaphore leaks. When WaitAsync throws OperationCanceledException, the semaphore is not acquired, so no release is needed. When WaitAsync succeeds, the Release() call after the try-catch executes normally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.csPlugins/Flow.Launcher.Plugin.Program/Programs/Win32.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-09-28T03:59:59.693Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3770
File: Plugins/Flow.Launcher.Plugin.Explorer/Views/RenameFile.xaml.cs:75-80
Timestamp: 2025-09-28T03:59:59.693Z
Learning: In Flow.Launcher's Explorer plugin rename dialog (RenameFile.xaml.cs), the dialog should close unconditionally after calling RenameThing.Rename(), even on validation failures, because RenameThing.Rename() displays error messages via popup notifications. This is the intended UX design.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
Main(18-551)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (1)
320-320: LGTM!The fix correctly awaits the async method directly instead of wrapping in
Task.Run, and uses the named parameterresetCache: truefor clarity as requested.Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs (1)
799-799: LGTM!The fix correctly awaits the async method directly instead of wrapping in
Task.Run, and uses the named parameterresetCache: truefor clarity.Plugins/Flow.Launcher.Plugin.Program/Main.cs (3)
337-366: LGTM!The
resetCacheparameter is cleanly integrated. Cache is reset conditionally based on the parameter value while maintaining proper lock handling and exception logging.
368-397: LGTM!Consistent implementation with
IndexWin32ProgramsAsync. The conditional cache reset pattern is applied correctly.
481-525: LGTM - correctly fixes the root cause.The fix properly addresses the original bug:
- Uses
FirstOrDefaultinstead ofFirst(lines 491, 509)- Adds null checks before accessing the program (lines 492, 510)
- Returns
falsefor already-disabled or not-found programs- Returns
trueonly when a program is actually disabledThe
resetCache: falseparameter is appropriate since the caller handles cache reset based on the return value.
| var disabled = await DisableProgramAsync(program); | ||
| if (disabled) | ||
| { | ||
| ResetCache(); |
There was a problem hiding this comment.
How does the ResetCache call work when we are resetting multiple times ? For example user disables one program after another.
There was a problem hiding this comment.
How does the ResetCache call work when we are resetting multiple times ? For example user disables one program after another.
No idea about what you said. Every time one program is disabled, it will invoke a task to add the program in disable list and reindex.
There was a problem hiding this comment.
How does the ResetCache call work when we are resetting multiple times ? For example user disables one program after another.
It will reset the cache every time you disable a program.
The one elegant way is to only remove the cache entry of the disabled one. But yeah calling resetting cache one by one is acceptable imo.
There was a problem hiding this comment.
What I am mean is you have IndexProgramsAsync making two concurrent calls to ResetCache() via IndexWin32ProgramsAsync and IndexUwpProgramsAsync. This could lead to:
- Double disposal
- Final cache ends up as the last writer and the other cache is lost and leaked or disposed according to timing.
- Disposing the old MemoryCache while another is concurrently reading/writing to it can cause error behavior.
Co-authored-by: Jeremy Wu <jeremy24wu@gmail.com>
Co-authored-by: Jeremy Wu <jeremy24wu@gmail.com>
Co-authored-by: Jeremy Wu <jeremy24wu@gmail.com>
Co-authored-by: Jeremy Wu <jeremy24wu@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
451-470: Consider providing user feedback when program cannot be disabled.The success message is now correctly conditional on
DisableProgramAsyncreturningtrue. However, when it returnsfalse(program already disabled or not found), the user receives no feedback. They click "Hide" and nothing visibly happens.if (disabled) { ResetCache(); Context.API.ShowMsg( Context.API.GetTranslation("flowlauncher_plugin_program_disable_dlgtitle_success"), Context.API.GetTranslation( "flowlauncher_plugin_program_disable_dlgtitle_success_message")); } + else + { + // Program was already disabled or not found + Context.API.ShowMsg( + Context.API.GetTranslation("flowlauncher_plugin_program_disable_dlgtitle_error"), + Context.API.GetTranslation("flowlauncher_plugin_program_already_disabled")); + } Context.API.ReQuery();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs(7 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-16T10:48:30.573Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-09-04T11:52:29.096Z
Learnt from: jjw24
Repo: Flow-Launcher/Flow.Launcher PR: 3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.096Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-09-28T03:59:59.693Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3770
File: Plugins/Flow.Launcher.Plugin.Explorer/Views/RenameFile.xaml.cs:75-80
Timestamp: 2025-09-28T03:59:59.693Z
Learning: In Flow.Launcher's Explorer plugin rename dialog (RenameFile.xaml.cs), the dialog should close unconditionally after calling RenameThing.Rename(), even on validation failures, because RenameThing.Rename() displays error messages via popup notifications. This is the intended UX design.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-11-06T12:57:11.574Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4072
File: Plugins/Flow.Launcher.Plugin.Program/Main.cs:100-111
Timestamp: 2025-11-06T12:57:11.574Z
Learning: In Flow Launcher's Program plugin Main.cs QueryAsync method, the semaphore pattern where WaitAsync(token) is inside a try-catch that only catches OperationCanceledException, followed by an explicit Release() call after the try-catch block, is safe and doesn't cause semaphore leaks. When WaitAsync throws OperationCanceledException, the semaphore is not acquired, so no release is needed. When WaitAsync succeeds, the Release() call after the try-catch executes normally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (2)
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (1)
Task(295-323)Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs (1)
Task(789-801)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (4)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (4)
337-366: LGTM!The
resetCacheparameter allows callers to control cache invalidation appropriately. When called from directory/package watchers (per the relevant snippets),resetCache: trueensures proper cache invalidation. When called fromDisableProgramAsync,resetCache: falseavoids redundant resets since the caller handles it.
368-397: LGTM!Consistent implementation with
IndexWin32ProgramsAsync. The conditional cache reset pattern is applied uniformly.
399-412: LGTM!Full reindex correctly uses
resetCache: trueto ensure fresh query results.
481-525: Core bug fix looks correct.The use of
FirstOrDefaultwith null checks (lines 491-492, 509-510) properly addresses the original issue whereFirst()would throw when the program wasn't found. The return value now correctly indicates whether a program was actually disabled.The flow is sound:
- Check if already disabled → return false
- Find in UWP list → disable and return true
- Find in Win32 list → disable and return true
- Not found → return false
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (3)
337-397: Consider consolidating cache reset logic.Both
IndexWin32ProgramsAsyncandIndexUwpProgramsAsyncare called concurrently fromIndexProgramsAsync(lines 401-411) withresetCache: true. This results in two calls toResetCache()executing concurrently, creating two new cache instances where one is immediately swapped out and disposed. While the atomic swap prevents corruption, it's wasteful.Consider moving the cache reset to
IndexProgramsAsyncafter both tasks complete, and always passresetCache: falseto the individual indexing methods.🔎 Proposed refactor
public static async Task IndexProgramsAsync() { var win32Task = Task.Run(async () => { - await Context.API.StopwatchLogInfoAsync(ClassName, "Win32Program index cost", () => IndexWin32ProgramsAsync(resetCache: true)); + await Context.API.StopwatchLogInfoAsync(ClassName, "Win32Program index cost", () => IndexWin32ProgramsAsync(resetCache: false)); }); var uwpTask = Task.Run(async () => { - await Context.API.StopwatchLogInfoAsync(ClassName, "UWPProgram index cost", () => IndexUwpProgramsAsync(resetCache: true)); + await Context.API.StopwatchLogInfoAsync(ClassName, "UWPProgram index cost", () => IndexUwpProgramsAsync(resetCache: false)); }); await Task.WhenAll(win32Task, uwpTask).ConfigureAwait(false); + ResetCache(); }
463-482: Move ReQuery inside the success block and add failure feedback.Currently,
ReQuery()is called unconditionally (line 476) even whenDisableProgramAsyncreturns false (program not found or already disabled). This triggers an unnecessary requery when nothing changed. Additionally, users receive no feedback when the disable operation fails.🔎 Proposed fix
try { var disabled = await DisableProgramAsync(program); if (disabled) { ResetCache(); Context.API.ShowMsg( Context.API.GetTranslation("flowlauncher_plugin_program_disable_dlgtitle_success"), Context.API.GetTranslation( "flowlauncher_plugin_program_disable_dlgtitle_success_message")); + Context.API.ReQuery(); + } + else + { + Context.API.ShowMsg( + Context.API.GetTranslation("flowlauncher_plugin_program_disable_dlgtitle_error"), + "Program not found or already disabled"); } - Context.API.ReQuery(); }
509-510: Fire-and-forget reindexing could hide failures.The reindex operations (lines 509, 527) are launched without awaiting them, meaning any exceptions during reindexing are unobserved and silently ignored. While this doesn't cause the hide-item bug (since
program.Enabledis set immediately andQueryAsyncfilters byEnabled), it could hide legitimate indexing failures.For better observability, consider logging a message when launching the reindex task, or wrapping the lambda in a try-catch that logs exceptions.
🔎 Example improvement for error visibility
- _ = Task.Run(() => IndexUwpProgramsAsync(resetCache: false)); + _ = Task.Run(async () => + { + try + { + await IndexUwpProgramsAsync(resetCache: false); + } + catch (Exception ex) + { + Context.API.LogException(ClassName, "Background UWP reindex after disable failed", ex); + } + });Apply similar pattern for Win32 reindex at line 527.
Also applies to: 527-528
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-10-16T10:48:30.573Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-09-04T11:52:29.096Z
Learnt from: jjw24
Repo: Flow-Launcher/Flow.Launcher PR: 3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.096Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-09-28T03:59:59.693Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3770
File: Plugins/Flow.Launcher.Plugin.Explorer/Views/RenameFile.xaml.cs:75-80
Timestamp: 2025-09-28T03:59:59.693Z
Learning: In Flow.Launcher's Explorer plugin rename dialog (RenameFile.xaml.cs), the dialog should close unconditionally after calling RenameThing.Rename(), even on validation failures, because RenameThing.Rename() displays error messages via popup notifications. This is the intended UX design.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-11-06T12:57:11.574Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4072
File: Plugins/Flow.Launcher.Plugin.Program/Main.cs:100-111
Timestamp: 2025-11-06T12:57:11.574Z
Learning: In Flow Launcher's Program plugin Main.cs QueryAsync method, the semaphore pattern where WaitAsync(token) is inside a try-catch that only catches OperationCanceledException, followed by an explicit Release() call after the try-catch block, is safe and doesn't cause semaphore leaks. When WaitAsync throws OperationCanceledException, the semaphore is not acquired, so no release is needed. When WaitAsync succeeds, the Release() call after the try-catch executes normally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (2)
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (1)
Task(295-323)Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs (1)
Task(789-801)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (2)
414-431: LGTM! Atomic swap correctly prevents concurrent disposal issues.The atomic swap using
Interlocked.Exchangeensures that when multiple threads callResetCache()concurrently, each receives a distinct old cache instance to dispose. This eliminates the risk of double-disposal or lost cache references that could occur with a naive implementation.
503-511: LGTM! Bug fix correctly uses FirstOrDefault instead of First().The change from
First()toFirstOrDefault()(lines 503, 521) is the core fix for issue #4139. Previously,First()would throw anInvalidOperationExceptionwhen the program wasn't found in the collection, causing the hide operation to fail.FirstOrDefault()returns null when no match is found, allowing the code to gracefully handle the not-found case and return false.The lock acquisition, program state updates, and early-exit checks are all correctly implemented.
Also applies to: 521-529
The original problem is the call of First() method. I fix this with null check.
Additionally, this pull request refactors and improves the program indexing and disabling logic in the
Flow.Launcher.Plugin.Programplugin. The main changes introduce more robust cache management, better handling of program disabling, and improved asynchrony and error handling.Improvements to indexing and cache management:
resetCacheparameter (defaulting totrue) to bothIndexWin32ProgramsAsyncandIndexUwpProgramsAsync, allowing more control over when the cache is reset. The cache is only reset if this parameter istrue. [1] [2] [3] [4]resetCache: truefor correct cache invalidation. [1] [2] [3]Improvements to disabling programs:
DisableProgramAsyncto return aboolindicating whether a program was actually disabled, and to only trigger reindexing for the relevant program type (UWP or Win32).Code quality and safety:
catalogfield inUWPPackage.csfrom nullable to non-nullable, reflecting that it is always initialized before use.These changes improve the efficiency, reliability, and user experience of program management in the plugin.
Fixes #4139
Tested